Skip to content

style(cfg): apply review items in #385 for gen-config#392

Merged
minrk merged 1 commit intoipython:masterfrom
ankostis:gencfg_review
May 5, 2017
Merged

style(cfg): apply review items in #385 for gen-config#392
minrk merged 1 commit intoipython:masterfrom
ankostis:gencfg_review

Conversation

@ankostis
Copy link
Copy Markdown
Contributor

@ankostis ankostis commented Apr 13, 2017 •
edited
Loading

Minor changes in the formatting of the generated sample configuration-file,
as reviewed by @ankostis on #385.

The differences in the generated config-file are shown below for this code:

import traitlets.config as trtc
import traitlets as trt

class C(trtc.Configurable):
    a = trt.Enum(['aa', 'BB'], help='1st help line.\n\nOther lines.').tag(config=True)

class A(trtc.Application, C):
    pass

print(A().generate_config_file())

Wihtout this PR:

#------------------------------------------------------------------------------
# A(Application, C) configuration
#------------------------------------------------------------------------------
## This is an application.

## 1st help line.
#  See also C.a
# c.A.a = traitlets.Undefined

WITH this PR:

THIS PR:
##------------------------------------------------------------------------------
## A(Application, C) configuration
##------------------------------------------------------------------------------
## This is an application.

## 1st help line.
#  See also: C.a
#c.A.a = traitlets.Undefined

minrk
minrk reviewed May 3, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think only the colon on "See also" is an improvement over what is there now, so can we omit the other two changes?

Comment thread traitlets/config/configurable.py Outdated
)

s = "# %s(%s) configuration" % (cls.__name__, parent_classes)
s = "## %s(%s) configuration" % (cls.__name__, parent_classes)
Copy link
Copy Markdown
Member

@minrk minrk May 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep single #, not double here and above.

Copy link
Copy Markdown
Contributor Author

@ankostis ankostis May 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Comment thread traitlets/config/configurable.py Outdated
lines.append('# See also: %s.%s' % (defining_class.__name__, name))

lines.append('# c.%s.%s = %s' % (cls.__name__, name, default_repr))
lines.append('#c.%s.%s = %s' % (cls.__name__, name, default_repr))
Copy link
Copy Markdown
Member

@minrk minrk May 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The space between # and c are helpful, let's keep them here.

Copy link
Copy Markdown
Contributor Author

@ankostis ankostis May 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to keep it?

Otherwise, then the last code line does not differ very little from comments immediately above, and
it needs 2 key strokes to enable the code.

Note, it was like these before #385 (actually i believe it was me that had passed this change in the first place, and had been accepted).

Copy link
Copy Markdown
Member

@minrk minrk May 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the space. It's generally recommended to have a space between the comment symbol and the comment itself. Plus, editors that have a 'comment/uncomment' action generally deal with this in a single keystroke as well.

@ankostis
Copy link
Copy Markdown
Contributor Author

ankostis commented May 4, 2017

OK, kept only See also: change and rebased on latest master.

Regarding the code-lines that now have 1 space between the # char,
it is important that we preserve some difference between the comment-lines.
Currently, the comment-lines start with 2 spaces after #.

So the following expression can collect all code-lines (and possibly some user-edited comments, but that's ok):

sed -n '/^# \w/p'  config.py

@minrk minrk merged commit e83b4de into ipython:master May 5, 2017
@minrk minrk added this to the 5.0 milestone May 5, 2017
@Carreau Carreau added 5.0-re-review Need to re-review for potential API impact changes. 5.0-no-incidence change that has noincidence on 5.0 compat (eg: doc) and removed 5.0-re-review Need to re-review for potential API impact changes. labels Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@minrk minrk minrk left review comments

Assignees

No one assigned

Labels

5.0-no-incidence change that has noincidence on 5.0 compat (eg: doc)

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

3 participants

@ankostis @minrk @Carreau